-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add user map of institutions (WIP) #773
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly structural stuff. LMK when the map is working and I'll take another look
frontend/vite.config.ts
Outdated
@@ -54,4 +54,7 @@ export default defineConfig({ | |||
}, | |||
}, | |||
}, | |||
optimizeDeps: { | |||
exclude: ['jeep-sqlite/loader'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this doing?
package-lock.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't need to be here
frontend/package.json
Outdated
@@ -14,7 +14,8 @@ | |||
"lint:fix": "eslint --fix . --ext .vue,.js,.ts --fix --ignore-path .gitignore", | |||
"style": "prettier --check src/", | |||
"style:fix": "prettier --write src/", | |||
"tls": "npm run type-check && npm run test && npm run lint && (npm run style || npm run style:fix)" | |||
"tls": "npm run type-check && npm run test && npm run lint && (npm run style || npm run style:fix)", | |||
"dev": "vite --force" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package.json and package-lock.json conflict with the main branch. Probably easiest to revert the lockfile, rebase, and rebuild
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just for testing, right? I'd leave this untracked since it won't get used in production
frontend/src/assets/isocountries.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is used by anything, wouldn't belong in the client assets/ if it was
d0164e6
to
8db6723
Compare
- fix refeernce to institutionData - add proj4 typescript typedefs
- remove dead imports - delete_ror_affiliations seems unnecessary, added a --force option to update_ror_affiliations instead - return dict object for update instead of tuple values in update_ror_affiliations.lookup_ror_id - fix invalid test_metrics method call ref
- use requests.Session context manager instead of caching on the Command instance - add with_affiliations() queryset method to MemberProfile and other minor hygiene
pull a few more details from the ror api including wikidata ids for future semantic web support, acronyms, and aliases add ror api url to settings should probably make a ror api service at some point but seemed overkill and also the ror schema might benefit from codemeticulous changes in the github mirroring PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should capture the additional metadata from RoR when we make new entries. This should be an easy fix and I can do it sometime in the next few hrs along with (hopefully) finding a better layout for the metrics page
Everything else looks good. Thanks @CharlesSheelam and @alee for fixing.
main ror api client remains on the client-side. The mgmt commands dealing with the ror api on the server are meant as one-offs so I'm not sure if there is a benefit to moving this to a wrapper api on the server * move to ROR api v2 * slightly adjusted what is stored for affiliations (the whole primary location object from ror instead of picking out fields)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @sgfost & @CharlesSheelam ! deployed on staging and lgtm
data = response.json() | ||
print(".", end="", flush=True) | ||
return { | ||
"name": data["name"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's other data we should be storing from the ROR API while we're running this lookup:
- acronyms
- labels
- wikipedia_url
- wikidata
- country
also we should start using the ROR v2 API https://ror.readme.io/v2/docs/rest-api though this will change some of the fields that are getting pulled
https://ror.readme.io/v2/docs/schema-v2#forthcoming-v20-example-1
-seperated codebase and codebase release total counts on the metrics table
-set up the backend to gather institution data for the user map